Repository navigation
test(node): pin task list limit ceiling and default on the cursor handler - #572
Conversation
…limit returns 500 with raw DB error
…dler Port the #405 regression coverage onto the current cursor-paginated handler: i64::MAX clamps to the 200 ceiling and an omitted limit falls back to the 50-row default. Negative-limit behavior is already pinned by negative_limit_returns_empty.
|
Warning Review limit reachedThis review ran on the open-source allowance, not this organization's plan, because the pull request author doesn't have an assigned seat. Waiting won't change this — ask an organization admin to assign them a seat, or add seats in Billing if every seat is already assigned, then retry. Next included review available in 55 minutes. View limit details
No actionable comments were generated in the recent review. Walkthrough
Merge Risk:
|
|
@coderabbitai full review |
Action not completed
|
jatmn
left a comment
There was a problem hiding this comment.
🟢 Thanks for the contribution. I do not see any actionable issues from my review.
Merge readiness
- Supersedes #405: Close or link #405 when this lands so the duplicate open PR does not linger.
- Checks: At review time,
test (stable),test (beta), andAnalyze (rust)were still pending;fmt + clippy,MSRV (Rust 1.91), andbuild --releasehad passed. Confirm the remaining required jobs are green before merge (GitHubmergeStateStatuswasBLOCKEDwhile those ran). - CodeRabbit: Reported skipping on this head; no actionable automated review to reconcile yet.
Notes (non-blocking)
The new pins assert count only. Module tests full_page_advertises_has_more_and_enumerates_past_the_row_cap already guard echoed limit and has_more for over-ceiling requests (limit=500). Optional follow-up — not required for this PR — would mirror the contributor’s tasks array length assertions from the original #405 tests or reuse MAX_VISIBLE_TASKS literals in assertions for drift resistance.
Summary
Re-home of #405 (author: jialfaro), carried onto current main. The original clamp is already on main through the cursor-paginated
list_tasks; this branch keeps the contributor history and adds the two regression pins main still lacks: the numeric ceiling (200) and the omitted-limit default (50).Motivation & context
#399 reported that a negative
limitonGET /api/v1/tasksreturned 500 with a raw database error, and that the limit was unbounded. Both are already fixed on main:collect_visible_tasksclamps the requested limit to[0, 200]and negative-limit behavior is pinned bynegative_limit_returns_empty. The residual value in the original PR is its test contract: explicit coverage that an over-ceilinglimitreturns at most 200 rows and that an omitted limit falls back to 50.Kind of change
What changed
Contributor work (unchanged): the original clamp and tests written against the pre-cursor handler. The merge took main's side of
tasks.rswholesale, since the contributor's directlist_tasksclamp is superseded bycollect_visible_tasks.Maintainer commit:
limit_ceiling_caps_at_200: seeds 201 tasks, requestslimit=9223372036854775807(i64::MAX), assertscount == 200.omitted_limit_uses_default_50: seeds 60 tasks, omitslimit, assertscount == 50.How a reviewer can verify
cargo test -p gitlawb-node --bin gitlawb-node api::tasks::visible_tasks_testscovers the two new pins plusnegative_limit_returns_empty.collect_visible_tasks; this PR adds coverage, not behavior.Before you request review
cargo fmt --all -- --checkcleancargo clippy --locked --workspace --all-targets -D warningsclean (pre-push gate ran on this head)Protocol & signing impact
None: tests only, no behavior change.
Notes for reviewers
The implementation half of #405 is redundant on current main; the merge took main's side and this branch's delta is the two pins. Negative-limit coverage stays with the existing
negative_limit_returns_empty. Supersedes #405.Summary by CodeRabbit